Deploy `RRGraphBuilder` in RRGraph Clock Builder to replace the use of `rr_node_indices` #1801

tangxifan · 2021-07-14T21:20:41Z

Description

This PR focuses on updating RRGraph clock builder functions, where we use the refactored data structure RRGraphBuilder to replace the legacy data structure rr_node_indices.
This PR aims to eliminate the use of rr_node_indices in the RRGraph clock builder, as one step further in deprecating the legacy data structure.

Checklist:

Added a new API find_sink_nodes() to RRSpatialLookup
Eliminate the use of rr_node_indices for RRGraph Clock Builder
Delete obsoleted functions in rr_graph2.cpp which are replaced by built-in APIs in RRSpatialLookup

Related Issue

Motivation and Context

This pull request is a follow-up PR on the routing resource graph refactoring effort #1779

How Has This Been Tested?

After the previous PR #1779 , we start reworking all the source files that use the legacy data structure rr_node_indices in a high priority, in order to deprecate the legacy data structure as soon as possible.
Current statistics on the files that use rr_node_indices (in total there are 143 lines related):

./route/clock_connection_builders.cpp
./route/clock_network_builders.cpp
./route/clock_network_builders.h
./route/router_lookahead_map_utils.cpp
./route/rr_graph.cpp
./route/rr_graph2.cpp
./route/rr_graph2.h
./route/rr_graph_clock.cpp
./route/rr_graph_clock.h
./route/rr_graph_reader.cpp
./route/rr_graph_util.cpp
./route/rr_graph_uxsdcxx_serializer.h
./route/rr_graph_writer.cpp

This PR will remove the use in

./route/clock_connection_builders.cpp
./route/clock_network_builders.cpp
./route/clock_network_builders.h
./route/rr_graph_clock.cpp
./route/rr_graph_clock.h

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed

tangxifan · 2021-07-14T21:54:57Z

@mithro Not sure why the Yosys+ODINII strong tests always failed in a few second. The artifact log file is empty.

vaughnbetz

Thanks @tangxifan . Looks generally good, but I have some suggested changes / comments (none too major).

vaughnbetz · 2021-07-16T00:03:26Z

vpr/src/device/rr_spatial_lookup.cpp

+        return sink_nodes;
+    }
+
+    /* By default, we always added the channel nodes to the TOP side (to save memory) */


This comment looks wrong (talking about channels).

This code also points out why we should rework the underlying rr_node_indices implementation; it's strange to have a multi-dimensional array matrix where some dimensions must always be accessed with a specific index. Hopefully that is on your near-term agenda Xifan. Could put a TODO in to refactor that way, if you think such a comment would be a useful reminder.

Sorry for the typo. Just fixed it.
I think it will be a good time to rethink the rr_node_indices data structure once all the related APIs are replaced.
Actually, it is not too far. I expect only we need another 2 pull requests, before fully shadowing the current rr_node_indices.

I put a TODO to remind us during the refactoring. I will take a deep look into the rr_node_indices data structure and propose something.

vaughnbetz · 2021-07-16T00:06:19Z

vpr/src/device/rr_spatial_lookup.cpp

+    }
+
+    for (const auto& node : rr_node_indices_[SINK][x][y][node_side]) {
+        if (RRNodeId(node)) {


Could comment this to say you're inserting only the valid ids in the returned vector.

Agree. Added the comment.

vaughnbetz · 2021-07-16T00:06:54Z

vpr/src/device/rr_spatial_lookup.h

+     *
+     * Note: 
+     * - Return an empty list if there are no sinks at the given (x, y) location
+     * - The node list returned only contain valid ids


typo: contain -> contains

Thanks for pointing it out. Fixed.

vpr/src/route/clock_connection_builders.cpp

vaughnbetz · 2021-07-16T00:21:23Z

vpr/src/route/clock_connection_builders.cpp


-    return node_index;
+    return size_t(node_index);


Can we change the return type of this function to RRNodeId and remove the cast? (May have to change the calling function too some then).

Just checked the dependency of this API. It is used only by another API. It is simple change. Done.

vaughnbetz · 2021-07-16T00:23:46Z

vpr/src/route/clock_connection_builders.cpp

+    // However, since the SINK node has the same xhigh/xlow as well as yhigh/ylow, we can probably use a shortcut
+    for (int ix = rr_graph.node_xlow(node_index); ix <= rr_graph.node_xhigh(node_index); ++ix) {
+        for (int iy = rr_graph.node_ylow(node_index); iy <= rr_graph.node_yhigh(node_index); ++iy) {
+            node_lookup.add_node(node_index, ix, iy, rr_graph.node_type(node_index), rr_graph.node_ptc_num(node_index), SIDES[0]);


Looks like we use SIDES[0] to insert the sink node, and look for it later on the TOP side. Hence should we use TOP consistently or SIDES[0] consistently to make sure this is always in alignment? I assume SIDES[0] == TOP so this all works, but it still seems dangerous as if anyone ever edits the order of SIDES we'd subtly break code written this way.

Agree. We should not expose the side argument to developers because they may feel confused.
You are right on the SIDE[0] which is actually the TOP side. This convention has been there for a while (I do not know who introduced it). I am o.k. to change it if necessary.
Indeed, developers may mess up.
For example, they added a channel node to the BOTTOM side but in find_node(), it will always search the TOP side.

vtr-verilog-to-routing/vpr/src/device/rr_spatial_lookup.cpp

Lines 23 to 29 in c06800f

e_side node_side = side;

if (type == IPIN || type == OPIN) {

VTR_ASSERT_MSG(side != NUM_SIDES, "IPIN/OPIN must specify desired side (can not be default NUM_SIDES)");

} else {

VTR_ASSERT_SAFE(type != IPIN && type != OPIN);

node_side = SIDES[0];

}

However, I think it is not a good idea to force the developers to give a dummy side when addethe API. I modified the API so that the side option will have a default value (SIDE[0]) if not specified. In client functions, I remove all the usage of the SIDE[0] for non-IPIN/OPIN nodes.

vaughnbetz · 2021-07-16T00:24:31Z

vpr/src/route/clock_connection_builders.cpp

@@ -320,7 +319,7 @@ void ClockToPinsConnection::create_switches(const ClockRRGraphBuilder& clock_gra

                    //Create edges depending on Fc
                    for (size_t i = 0; i < clock_network_indices.size() * fc; i++) {
-                        clock_graph.add_edge(rr_edges_to_create, clock_network_indices[i], clock_pin_node_idx, arch_switch_idx);
+                        clock_graph.add_edge(rr_edges_to_create, clock_network_indices[i], size_t(clock_pin_node_idx), arch_switch_idx);


Can we change the function prototype to avoid the cast (send in an RRNodeId instead)?

See my reply in a previous comment on the t_rr_edge_info

vaughnbetz · 2021-07-16T00:28:24Z

vpr/src/route/clock_network_builders.cpp

+            //TODO: CHANX uses odd swapped x/y indices here. Will rework once rr_node_indices is shadowed
+            rr_graph_builder.node_lookup().add_node(chanx_node, iy, ix, rr_graph.node_type(chanx_node), rr_graph.node_ptc_num(chanx_node), SIDES[0]);
+        }
+    }

    return node_index;


Consider changing return type to RRNodeId, and directly making node_index an RRNodeId (and therefore wouldn't need chanx_node).

Actually, I believe, we can use the API add_node_to_all_locs() as you suggested in another PR #1800. These for loops will be replaced by a simple line:

rr_graph_builder.add_node_to_all_locs(chanx_node)

I leave a TODO comment here to remind me to finish the API replacement after PR #1800 is merged.
Let me know if this is fine for you.

vpr/src/route/clock_network_builders.cpp

vaughnbetz · 2021-07-16T00:36:46Z

Note: the Yosys+Odin-II failure is an expected, unrelated failure and is not gating.

…Add comments about TODO action items;

…void duplicated codes

vaughnbetz · 2021-07-18T18:33:35Z

Looks good; thanks.

tangxifan added 3 commits July 14, 2021 14:29

[VPR] Deploy RRGraphBuilder to clock network builder

c37643c

[VPR] Deploy RRSpatialLookup to clock connection builder

e6e2a2f

[VPR] Delete unused functions

8804edd

github-actions bot added the VPR VPR FPGA Placement & Routing Tool label Jul 14, 2021

tangxifan added 2 commits July 14, 2021 15:22

[VPR] Code format fix

3dfafbf

[VPR] Fix compiler warning

1f3c410

tangxifan requested review from vaughnbetz and hzeller July 15, 2021 17:50

vaughnbetz requested changes Jul 16, 2021

View reviewed changes

tangxifan added 2 commits July 17, 2021 16:48

[VPR] Remove the hard requirement on side option when adding a node; …

d4a76f6

…Add comments about TODO action items;

[VPR] Reworked RRSpatialLookup: added a private API find_nodes() to a…

ab9dc4e

…void duplicated codes

vaughnbetz merged commit 78cd631 into master Jul 18, 2021

vaughnbetz deleted the rr_graph_clock_refactor branch July 18, 2021 18:33

tangxifan mentioned this pull request Jul 19, 2021

Deprecate get_rr_node_indices() functions #1805

Merged

15 tasks

	e_side node_side = side;
	if (type == IPIN \|\| type == OPIN) {
	VTR_ASSERT_MSG(side != NUM_SIDES, "IPIN/OPIN must specify desired side (can not be default NUM_SIDES)");
	} else {
	VTR_ASSERT_SAFE(type != IPIN && type != OPIN);
	node_side = SIDES[0];
	}

Deploy RRGraphBuilder in RRGraph Clock Builder to replace the use of rr_node_indices #1801

Deploy RRGraphBuilder in RRGraph Clock Builder to replace the use of rr_node_indices #1801

Uh oh!

Conversation

tangxifan commented Jul 14, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

tangxifan commented Jul 14, 2021

Uh oh!

vaughnbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tangxifan Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vaughnbetz commented Jul 16, 2021

Uh oh!

vaughnbetz commented Jul 18, 2021

Uh oh!

Uh oh!

Deploy `RRGraphBuilder` in RRGraph Clock Builder to replace the use of `rr_node_indices` #1801

Deploy `RRGraphBuilder` in RRGraph Clock Builder to replace the use of `rr_node_indices` #1801

tangxifan Jul 17, 2021 •

edited

Loading